Fix handling of parsable types in validations generator #61728
Conversation
ghost
added
the
needs-area-label
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the handling of parsable types in the validations generator by adding additional known type data and refactoring the generator code.
- Added new well-known types (e.g. JsonDerivedTypeAttribute, DataAnnotations attributes) to support enhanced validations.
- Updated tests and snapshots to cover parsable properties.
- Refactored generator parsers and extensions to replace RequiredSymbols with WellKnownTypes.
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Shared/RoslynUtils/WellKnownTypeData.cs | Added new entries to support additional attributes. |
| src/Http/Http.Extensions/test/ValidationsGenerator/snapshots/... | Updated expected test snapshots to reflect changes. |
| src/Http/Http.Extensions/test/ValidationsGenerator/ValidationsGenerator.Parsable.cs | Added tests to validate parsable property handling. |
| src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.ValidationsGenerator/ValidationsGenerator.cs | Simplified generator initialization by removing unnecessary symbols resolution. |
| Other generator/parsers files | Refactored to use WellKnownTypes, ensuring proper resolution of types. |
Files not reviewed (1)
- src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.ValidationsGenerator/Microsoft.AspNetCore.Http.ValidationsGenerator.csproj: Language not supported
captainsafia
added
area-minimal
| // Extract validatable types discovered in members of this type and add them to the top-level list. | ||
| var members = ExtractValidatableMembers(typeSymbol, requiredSymbols, ref validatableTypes, ref visitedTypes); | ||
| ImmutableArray<ValidatableProperty> members = []; | ||
| if (ParsabilityHelper.GetParsability(typeSymbol, wellKnownTypes) is Parsability.NotParsable) |
There was a problem hiding this comment.
This and the change in ValidationsGenerator are the only real changes in this PR?
And this is removing TryParseable types from the validatable types? But somehow they are still validatable?
There was a problem hiding this comment.
This and the change in ValidationsGenerator are the only real changes in this PR?
Yep, this is the main change. All the WellKnownType stuff is a reaction to the fact that this API needs it.
And this is removing TryParseable types from the validatable types? But somehow they are still validatable?
We'll still generate ValidatablePropertyInfo instances for them so that we'll validate attributes on the properties. This change does mean that we won't validate properties with validation attributes inside the parsable types. None of the properties in the case below get validated. I'd have to check how MVC handles this...if it does...
The right pattern hear might be to require that custom parsable types implement IValidatableObject to handle validation.
public class Person
{
[Required]
[StringLength(100, MinimumLength = 2)]
public string FirstName { get; private set; } = string.Empty;
[Required]
[StringLength(100, MinimumLength = 2)]
public string LastName { get; private set; } = string.Empty;
[Range(0, 120)]
public int Age { get; private set; }
// Static TryParse method
public static bool TryParse(string input, out Person? person) { }
}There was a problem hiding this comment.
Update: it looks like MVC doesn't validate properties on a TryParse by default:
#:sdk Microsoft.NET.Sdk.Web
#:property TargetFramework net10.0
using Microsoft.AspNetCore.Mvc;
using System.ComponentModel.DataAnnotations;
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddControllers();
var app = builder.Build();
app.MapControllers();
app.Run();
[ApiController]
[Route("[controller]")]
public class PersonController : ControllerBase
{
// This endpoint binds PersonInput from query string
[HttpGet]
public IActionResult CreatePerson([FromQuery] PersonInput personInput)
{
if (!ModelState.IsValid)
{
return BadRequest(ModelState);
}
return Ok(new { Message = $"Created person: {personInput.FirstName} {personInput.LastName}, Age {personInput.Age}" });
}
}
public class PersonInput
{
[Required]
[StringLength(100, MinimumLength = 2)]
public string FirstName { get; init; } = string.Empty;
[Required]
[StringLength(100, MinimumLength = 2)]
public string LastName { get; init; } = string.Empty;
[Range(0, 120)]
public int Age { get; init; }
// TryParse for parsing raw CSV strings (no validation inside)
public static bool TryParse(string input, out PersonInput? person)
{
person = null;
if (string.IsNullOrWhiteSpace(input))
return false;
var parts = input.Split(',', StringSplitOptions.TrimEntries);
if (parts.Length != 3)
return false;
if (!int.TryParse(parts[2], out int age))
return false;
person = new PersonInput
{
FirstName = parts[0],
LastName = parts[1],
Age = age
};
return true;
}
}$ curl http://localhost:5000/person?personInput=S,A,128
{
"message": "Created person: S A, Age 128"
}
There was a problem hiding this comment.
The right pattern hear might be to require that custom parsable types implement
IValidatableObjectto handle validation.
Does this work now, or are you saying we might want to make this work in the future?
There was a problem hiding this comment.
It doesn't work now. We would need to add a check to see if the type implements IValidatableObject and if so generate an IValidatableInfo instance for that member type.
2 participants
Closes #61525.